Skip to content

Conversation

@jaymzh
Copy link

@jaymzh jaymzh commented Mar 6, 2024

No description provided.

@ahmetb
Copy link
Member

ahmetb commented Mar 6, 2024

🤖 Beep beep! I’m a robot speaking on behalf of @ahmetb. 🤖


Thanks for submitting your kubectl plugin to Krew!
One of the krew-index maintainers will review it soon. Note that the reviews for new plugin submissions may take a few days.

In the meanwhile, here are a few tips to make your plugin manifest better:

  • Make sure your plugin follows the best practices.
  • Eliminate redundant wording form shortDescription (it should be max 50 characters).
  • Try to word wrap your description to 80-character lines (no usage examples, please).

Thanks for your patience!
/kind new-plugin

@k8s-ci-robot k8s-ci-robot requested a review from chriskim06 March 6, 2024 06:15
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jaymzh
Once this PR has been reviewed and has the lgtm label, please assign chriskim06 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 6, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: jaymzh / name: Phil Dibowitz (c1eae4b)

@k8s-ci-robot k8s-ci-robot added kind/new-plugin cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 6, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @jaymzh!

It looks like this is your first PR to kubernetes-sigs/krew-index 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew-index has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 6, 2024
@jaymzh jaymzh force-pushed the add-kubectl-daemons branch 5 times, most recently from bd83ebb to f75ee3d Compare March 6, 2024 06:27
Signed-off-by: Phil Dibowitz <phil@ipom.com>
@jaymzh jaymzh force-pushed the add-kubectl-daemons branch from f75ee3d to c1eae4b Compare March 6, 2024 06:28
@jaymzh
Copy link
Author

jaymzh commented Mar 9, 2024

Hey @chriskim06 - do you know roughly when you might get to this? Not trying to rush you, just try get a sense of the queue length. :) Thanks!

@ahmetb
Copy link
Member

ahmetb commented Mar 9, 2024

Sorry for the review latency. It takes a little while when we're busy with day jobs. :) I will take a look.

@chriskim06
Copy link
Member

sorry as well, i dont get a notification unless im assigned so didnt see this till you @'d me. ill also try to take a look in a bit

@chriskim06
Copy link
Member

this doesnt seem to really extend kubectl but more seems like a wrapper/shortcuts around existing kubectl functionality. most of these things seem accomplishable with labels and/or --field-selector spec.nodeName=.... it does remove needing to know labels to search with but we typically exclude these types of plugins from the central krew-index.

your plugin can still be distributed with krew through a custom index

@jaymzh
Copy link
Author

jaymzh commented Mar 9, 2024

this doesnt seem to really extend kubectl but more seems like a wrapper/shortcuts around existing kubectl functionality. most of these things seem accomplishable with labels and/or --field-selector spec.nodeName=.... it does remove needing to know labels to search with but we typically exclude these types of plugins from the central krew-index.

I don't know that I agree with that. While it's convention that people stick app=<daemonset> labels on their daemonset pod templates, there's nothing stopping someone from making, say, a deployment with the same name and the same label. And there's no, AFAIK, way to query based on controlling object. But that's what you really want, owner-Kind == "DaemonSet" && owner.Name == <whatever>). Further, there's no way I know to say "show me all pods from daemonsets on this node."

Kubectl is optimized for users of k8s, not administrators of large fleets that happen to run k8s. When you're debugging hosts, or trying to track down a odd performance issue, or trying to develop better DS's, this sort of functionality is really useful. This feels like it fits well within 'extending kubectl" to me.

@chriskim06
Copy link
Member

While it's convention that people stick app= labels on their daemonset pod templates, there's nothing stopping someone from making, say, a deployment with the same name and the same label

while this is technically true, i dont know how common this is in practice. if this were the only label on both the deployment and daemonset's pod template you wouldnt be able to define a service targeting just one or the other (not that you would always need that, but just a potential issue i could think of). kubernetes docs also suggest using multiple distinguishing labels.

there's no way I know to say "show me all pods from daemonsets on this node."

ya i guess thats true in the single label example, but if there are distinguishing labels i would just do a kubectl get pods -l 'app in (daemonset-a, daemonset-b),...' --field-selector spec.nodeName=....

Kubectl is optimized for users of k8s, not administrators of large fleets that happen to run k8s

this is just my opinion but if youre responsible for a cluster, i think theres an implicit expectation of at least some kubectl knowledge. this is a kubectl plugin that the administrator would be using so i feel like that expectation isnt completely off base.


we typically deny things along the line of kubectl get ... | <do something>. while i get this isnt completely like that, it feels sort of in that ballpark to me. @ahmetb do you have any thoughts on this?

@ahmetb
Copy link
Member

ahmetb commented Mar 10, 2024

Yeah, I also have doubts about the operational model. The readme cites finding daemonset pods on node, which can be done using kubectl pods-on -D <NODE>, and also mentions deleting a DaemonSet pod (which feels like a very rare operation, as the daemonset-controller would recreate it anyway).

Similarly, other subcommands like daemonset <delete|get|describe> don't seem to be any different than the generic kubectl commands that already exist? As for the logs subcommand, we have quite a few log plugins here and in the rest of the ecosystem about tailing logs for ~everything (not just limited to DaemonSets).

In my opinion, the plugin is rather repetitive of what kubectl has and I'm not sure if it adds anything significant on top. I also recommend publishing this in a custom index.

/kind gray-area

@jaymzh
Copy link
Author

jaymzh commented Mar 10, 2024

Wait, I thought you said that plugins that only do the equivalent of --field-selector spec.nodeName=.... aren't usually accepted, but isn't that roughly exactly what pods-on is? That can definitely be done in a single kubectl command, where as what kubectl-daemons does often is collapsing several commands into one far more efficient one.

I can go make a custom index - it's just another git repo. It just feels like this fits pretty well inline with many other plugins in the primary index.

@jaymzh
Copy link
Author

jaymzh commented Mar 10, 2024

Also,

and also mentions deleting a DaemonSet pod (which feels like a very rare operation, as the daemonset-controller would recreate it anyway).

Actually there's many reasons to do this:

  • The pod failed to do what it was supposed to on a single node (or a handful of nodes with no clear correlation) (say setup a driver or device), and you changed something environmentally and want to see if re-running it will do what you expect before making a wider / more automated chagne
  • You run your daemonsets with an update strategy of onDelete
  • You want to test a single node, so you change a daemonset policy to onDelete briefly, then delete a pod. This is one of those times where doing "delete " then "get pods | grep .. | gre.. | grep... " then "logs $newpod" is an extra PITA.

@ahmetb
Copy link
Member

ahmetb commented Mar 10, 2024

I believe kubectl pods-on does a little bit more than the command you listed as it is primarily meant for working beyond a single node queries.

I think we can certainly revisit this plugin when it has more differentiating functionality and less overlap with what's already in kubectl.

@jaymzh jaymzh closed this Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gray-area kind/new-plugin size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants